-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Checks to ensure Cluster name unique and required #13798
Conversation
@alena1108 review pls. |
9ec8345
to
278d871
Compare
|
||
name, ok := data["name"].(string) | ||
if !ok { | ||
return nil, errors.New("Cluster name is required") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prachidamle do we need this extra check? Given that required attribute is already specified as norman annotation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alena1108 yeah we need this check since the API request data is sent in a map and not as the Cluster object, so need to check the field to avoid panics while casting to string
return nil, errors.New("Cluster name is required") | ||
} | ||
|
||
r.mu.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code should be invoked on the update as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the update method
278d871
to
869e6b3
Compare
r.mu.Lock() | ||
defer r.mu.Unlock() | ||
|
||
var clusters []managementv3.Cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should perhaps extract the code into common method. From line 39 to line 51
|
||
func (r *Store) Create(apiContext *types.APIContext, schema *types.Schema, data map[string]interface{}) (map[string]interface{}, error) { | ||
|
||
name, ok := data["name"].(string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should extract lines 31-34 into common method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
869e6b3
to
ca4c885
Compare
clusterName = "" | ||
} | ||
|
||
updatedName, ok := data["name"].(string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we always expect the name in the update request? does UI send it all the time? if so, may be it's something we can check at the very top of the method, to save on querying the existing cluster if the name is missing. Also it can be extracted to its own method, so the call can replace line 31-34 in create
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah name is always sent in the data[] for the UPdate call, infact all properties are present in the data[]. So to know if name is getting updated, we would have to compare with the existing name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will swap the order of checking cluster name and pulling existing object. But just checking if clusterName is missing need not be a common method.
clusterName = "" | ||
} | ||
|
||
updatedName, ok := data["name"].(string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use convert library, otherwise there are chances of nil pointer. I believe it's convert.toString()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will check the library!
ca4c885
to
978a8e2
Compare
LGTM. @prachidamle could you remove this file from the vendor commit; usually binaries should not be included: |
978a8e2
to
e7d517d
Compare
#13075